Skip to content

[draft] port to wasm32-unknown-unknown & web - #8607

Open
makemeunsee wants to merge 46 commits into
helix-editor:masterfrom
makemeunsee:wasm32
Open

[draft] port to wasm32-unknown-unknown & web#8607
makemeunsee wants to merge 46 commits into
helix-editor:masterfrom
makemeunsee:wasm32

Conversation

@makemeunsee

@makemeunsee makemeunsee commented Oct 24, 2023

Copy link
Copy Markdown

Hi Helix team,

As mentioned in a discussion, here's a draft PR for my work to have (a subset of) Helix fully bundled as web app.

The README of the new crate helix-web lists current limitations and known issues.

Let me know if you'd like me to point out things here, or if you prefer to have a look first.

Also I'm looking now at re-enabling parsing with tree-sitter, I could compile some parsers to wasm so I'll try to embed them, instead of hot loading them, and use them. Not sure if I should push that work to the same branch though, it would make this PR even messier.

Demo

@archseer

Copy link
Copy Markdown
Member

@pascalkuthe continuing from #8588 (reply in thread), I think tokio added wasm support over the last year, running in a single thread in a service worker

Comment thread helix-core/src/history.rs
Comment on lines +7 to +10
#[cfg(target_arch = "wasm32")]
use instant::Instant;
#[cfg(not(target_arch = "wasm32"))]
use std::time::Instant;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than adding this conditional everywhere, let's re-export instant in the root of the helix-core crate, then use that everywhere else

insert_final_newline(doc, view);
}

#[cfg(feature = "dap_lsp")]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's separate out two feature flags, dap and lsp. That's what I did here master...gui

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally tried to rebase your gui branch but it was too much outdated. So I redid some of the work you did there in the first, and took a shortcut for these 2 features, knowing neither would work any time soon on wasm. But yeah, they should be distinct, I kinda regret not splitting them in the first place...

Ok(language)
}

#[cfg(not(target_arch = "wasm32"))]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this git/build handling seems like it could be split into a separate file/module then gated behind a single conditional

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is how you could add a WASM entrypoint for grammars too: add an alternative loader implementation that just uses an internal list of compiled-in grammars

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that's exactly what I'm attempting now; on the Helix side it looks quite doable, it's all the tree-sitter-* libs which will be more tedious to bring to wasm.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got syntax highlighting for rust, toml and regex to work, but it's very hacky:

  • hacked tree-sitter-c2rust so it uses instant instead of std::time
  • hacked C code of tree-sitter-* crates, for wasm compilation + dependency on hacked tree-sitter-c2rust; not big changes but still tedious and I dont see a way to automate the process. Maybe c2rusting the language libs, but I have no experience with it.

It's nice to see it working but it's quite a mess, so I'll keep it on the side for now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to use tree-sitter-c2rust rather than https://github.com/tree-sitter/tree-sitter/blob/master/lib/binding_web/README.md ? I think we should look at how the tree-sitter playground functions since it runs grammars via wasm (and it lazy loads them instead of compiling them into the main binary)

https://tree-sitter.github.io/tree-sitter/playground

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's interesting, I hadnt spotted they had the playground in Rust on wasm too.

It's puzzling, as tree-sitter actually compiles to wasm32-unknown-emscripten, but not to wasm32-unknown-unknown. And wasm32-unknown-emscripten is not compatible with wasm-bindgen: https://rustwasm.github.io/docs/wasm-bindgen/reference/rust-targets.html?highlight=emscripten#other-web-targets.

I'll take a look, it would be so much better this way.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so the playground is a Rust web server, serving emscripten wasm binaries and regular JS code. The hot loading is js/emscripten code, see https://github.com/tree-sitter/tree-sitter/blob/7c0cee70f55fdfe1667890d372441a85f3127ee6/lib/binding_web/binding.js#L1024-L1042

The Rust bindings compile to linux and wasm-emscripten, but not to wasm-unknown.

I dont see an easy way to interop between a wasm32-unknown app and wasm32-emscripten libs. Creating Rust bindings for tree-sitter-js and then writing a facade/adapter so Helix code can abstract over the native and web bindings? I got a headache just thinking about it 😅

Open to any ideas!

@archseer

Copy link
Copy Markdown
Member

Testing this out, xterm.js actually supports truecolor (but I guess doesn't advertise correctly). Setting :set true-color true I'm able to use the default theme. You probably want to add another override similar to windows for wasm:

#[cfg(windows)]
fn true_color() -> bool {
true
}

Comment on lines +21 to +25
- name: Install wasm-pack
uses: jetli/wasm-pack-action@v0.4.0
with:
version: 'latest'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The action does so little it would be nice to just replace with the official invocation: https://rustwasm.github.io/docs/wasm-bindgen/wasm-bindgen-test/continuous-integration.html#github-actions

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I picked this one as they advertise much faster (seconds vs. minutes) set up times, but we can use the official one sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

      - name: Install
        run: curl https://rustwasm.github.io/wasm-pack/installer/init.sh -sSf | sh

From the docs should be equally as fast, it figures out the target architecture and downloads the release (rather than cargo-install-ing and compiling from source)

@makemeunsee

Copy link
Copy Markdown
Author

I've just rebased onto master, while it's still manageable.

I'll try to ack the various comments over the next days, and point out of few other things to discuss.

but regular build is broken, see TODOs in Cargo.toml files and
rust-lang/cargo#12821
still many warnings and some functionalities are just commented out,
not feature gated
broken `cargo build`, fix by uncommenting
lines helix-editor#43-46 of helix-tui/Cargo.toml
broken `cargo build`, fix by uncommenting
lines helix-editor#43-46 of helix-tui/Cargo.toml & tokio features
`cargo build --no-default-features --target wasm32-unknown-unknown`:
works for helix-tui and dependencies
* feature gated vcs
* select instant::Instant or std::time::Instant or tokio::time::Instant
* redraw & idle timers, wip
can now run on wasm:
```
let app = Application::new(
        Args::default(),
        Config::default(),
        helix_core::config::default_syntax_loader(),
    );
```
preliminary xterm integration & xterm backend
- xterm rs bindings (partial, just enough for now)
- crossterm xterm backend (copied impl, not integrated)
- reduced tokio deps
@archseer

Copy link
Copy Markdown
Member

To get this merged I'd split the PR into parts:

  • Adding feature flags for lsp and dap, enabling by default
  • Adding wasm conditionals, and adding a wasm target to the CI build
  • helix-web, loader, tree-sitter changes etc.

With first two out of the way it would make the PR easier to rebase since the changes would be mostly scoped to helix-web

@makemeunsee

Copy link
Copy Markdown
Author

To get this merged I'd split the PR into parts:

* Adding feature flags for lsp and dap, enabling by default

* Adding wasm conditionals, and adding a wasm target to the CI build

* `helix-web`, loader, tree-sitter changes etc.

With first two out of the way it would make the PR easier to rebase since the changes would be mostly scoped to helix-web

Sounds like a plan!

@makemeunsee

Copy link
Copy Markdown
Author

Created #8638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants